-
Notifications
You must be signed in to change notification settings - Fork 8.2k
soc: xlnx: zynqmp: overhaul the Cortex-R MPU setup #79221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f55b2f4 to
657cc5f
Compare
bbf85d8 to
904c39f
Compare
8d3b5ad to
d752614
Compare
michalsimek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not upstream QEMU but AMD/Xilinx qemu fork. Is there any warning about it?
From my perspective having dtb as blob in source code is not a good idea. Isn't there a better way to put there dts and dtc will convert it as the part or build process.
Another reason is that it is hard to review what has been changed from between one dtb and another dtb.
|
You can refer to https://docs.zephyrproject.org/latest/contribute/bin_blobs.html for the policy around binary blobs in Zephyr. |
Thanks. |
|
Should there be a warning regarding the fact that Xilinx's fork of QEMU is used? If there were any functional issues with this version, this would become obvious with pretty much every PR as the QEMU targets are tested as part of the CI. Admittedly, there seem to be a few hiccups in there (aside from the afforementioned not caring about the black hole between the ATCM and the BTCM that exists on actual hardware, for example, there's just nothing in the clock configuration registers within the Zynq-7000's SLCR), but I'd only see a benefit in switching over if those issues were proven fixed in mainline QEMU. As this version is, along with other QEMU flavors, shipped with the SDK, it's not a case of having accidentially downloaded the wrong flavor of QEMU. Agreed, the case of QEMU only working with DTB's is inconvenient, as you can't easily diff. So adding the textual description that the RAM size was changed was the best that could be done here. However, I wouldn't see the DTB as a blob as in a firmware blob, for example. Your suggestion of having the device tree for QEMU in DTS format and converting it during the build process is rather something that I'd see in the area of the build framework / west integration, as QEMU is integrated all the way into (at least) west's run and debugserver commands. The link is likely board.cmake. I'd suggest writing an Enhancement Suggestion to the folks in charge of the build system. |
|
Re-triggering CI by means of closing/re-opening the PR to make sure it's still green before merging |
|
@ibirnbaum this branch might need to be rebased to main in order for the CI setup to work? |
f551a99 to
9f95427
Compare
Move the unmodified sram0 memory area declaration to this board's device tree due to its removal from the SoC device tree. Signed-off-by: Immo Birnbaum <[email protected]>
…d DT Move the unmodified sram0 memory area declaration to this board's device tree due to its removal from the SoC device tree. Signed-off-by: Immo Birnbaum <[email protected]>
Expand the RAM area beginning behind the BTCM to allow the use of a 64MB SRAM area starting at 0x4000000, which is the lowest possible 64MB area with a non-zero start address that doesn't overlap with the ATCM and BTCM memory areas. Signed-off-by: Immo Birnbaum <[email protected]>
Declare the sram0 area for this board to start at 0x4000000 and to be 64MB in size. The base address is the lowest possible multiple of 64MB which doesn't have its base address at 0, therefore preventing the mixed use of regular RAM and the ATCM/BTCM and the possible issues of having a 'black hole' between the ATCM and BTCM areas or the BTCM being completely disabled. Signed-off-by: Immo Birnbaum <[email protected]>
Remove the universal, unconditional declaration of the RAM area at the SoC level, due to: - the hardcoded base address 0 overlapping the exception vectors, the ATCM and the BTCM areas. - the availability of the BTCM not being guaranteed unconditionally (config pin dependant) - the possibility of having a 'black hole' between the ATCM and the BTCM depending on the operating mode of the R-cores cluster, which leads to a part of the text section being unavailable - qemu not properly implementing the configuration-dependant behaviour of the ATCM and BTCM areas. Signed-off-by: Immo Birnbaum <[email protected]>
9f95427 to
0161e50
Compare
update the directive that calculates the ROM region size bit mask for the MPU setup. Signed-off-by: Immo Birnbaum <[email protected]>
Overhaul the MPU region definitions that are being configured when the MPU is set up: - drop local attribute definitions in favor of those already provided in arm_mpu_v7m.h - actually tie the RAM region to the device tree - set up a (potentially overlapping) R/O region for .text and .rodata, which hasn't existed so far - Consider XIP Signed-off-by: Immo Birnbaum <[email protected]>
Apply the same modifications made to the ZynqMP's memory regions to the cortex_r8_virtual SoC which was mainlined while the fixes for the ZynqMP were being developed (minus the OCM mapping, as there's no indication that this type of memory was considered). The cortex_r8_virtual appears to be a stripped down copy of the old qemu_cortex_r5 codebase, therefore, the duplicated MPU regions have the same flaws as qemu_cortex_r5 or any actual ZynqMP-based target for that matter. Signed-off-by: Immo Birnbaum <[email protected]>
0161e50 to
c2d8e05
Compare
|
After a rebase against the current main, it turns out that in the meantime (2 months ago) someone at Renesas managed to use this ancient and wrong piece of code that is the old ZynqMP's MPU setup as basis for another of their SoCs - once again including local macros that are unnecessary (and they copied those without attribution of the original author), once again using hardcoded addresses without association to the device tree. Had they at least copied from AMD's more recent additions, they would have gotten a decent basis... I put the old MPU region size calculation formula for the R/O part of the RAM region back into the linker command file (not an issue as there is no name collision here) and I'll leave it to them to find out that it doesn't calculate the correct value. |
I recommend tagging the concerned pocs here so that they are aware of the issue. |
Will do, I'll also check if they can be reached via Discord alternatively. |



To this day, the MPU region definitions provided for the ZynqMP/UltraScale+'s Cortex-R cores, which are pretty old code by now, cause various problems on actual hardware, which qemu_cortex_r5 tends to ignore:
For the first two items, the macros CONFIG_SRAM_BASE_ADDRESS and REGION_SRAM_SIZE exist, which haven't been referenced prior.
Also, XIP wasn't considered and local macro definitions for region properties were used although global ones are already provided.
Therefore, the proposed overhaul changes the following:
I'm well aware that tying the RAM region size to the device tree directly contradicts #61008 which dates back to last year. However, I do believe that this was never a good idea to start with: generally opening up 2 GB of RAM address space in an SoC where the R-cores and the A-cores share the RAM just so that openamp SHMs could be effortlessly randomly placed at an address that may or may not be beyond the range actually used by the Zephyr image seems extremely risky when considering the possibility of parallel Linux operation on the A-cores. Also, this is a feature that is not generally used by everyone developing for the ZynqMP R-cores. Memory ranges for such specific use cases can be declared on a need-to-have basis in the respective target's device tree or in application-specifc overlays, the MPU setup will consider such additional region declarations automatically via mpu_configure_regions_from_dt(). If there's any board definitions or samples in-tree that make use of this feature on the ZynqMP, I can have a look on how to set up the SHM memory regions properly and add the fixes to this branch.